src: release context frame in AsyncWrap::EmitDestroy by Flarna · Pull Request #61995 · nodejs/node

Flarna · 2026-02-26T00:07:32Z

Release the async context frame in AsyncWrap::EmitDestroy to allow gc to collect it.

This is in special relevant for reused resources like HTTPParser otherwise they might keep ALS stores alive.

Release the async context frame in AsyncWrap::EmitDestroy to allow gc to collect it. This is in special relevant for reused resources like HTTPParser otherwise they might keep ALS stores alive.

codecov · 2026-02-26T02:40:59Z

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.64%. Comparing base (488a854) to head (3b54b9a).
⚠️ Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
src/async_wrap.cc	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61995      +/-   ##
==========================================
- Coverage   89.77%   89.64%   -0.13%     
==========================================
  Files         674      676       +2     
  Lines      205705   206252     +547     
  Branches    39449    39511      +62     
==========================================
+ Hits       184670   184897     +227     
- Misses      13280    13476     +196     
- Partials     7755     7879     +124

Files with missing lines	Coverage Δ
src/async_wrap.cc	`84.26% <83.33%> (-0.15%)`	⬇️

... and 68 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

addaleax · 2026-02-26T08:54:01Z

src/async_wrap.cc

+      HandleScope handle_scope(env()->isolate());
+      USE(object()->Set(env()->context(), env()->resource_symbol(), object()));
+    }
+    context_frame_.Reset();


Not for this PR, but do you know how it comes this is a v8::Global to begin with and not an internal field on object()?

Sorry, don't know. Maybe @Qard can chime in.

No particular reason, I guess. Probably could be an internal field. Just needed to have a context frame reference somewhere related to an AsyncWrap so it could do the restore between the before and after. It was just easier to store directly on the AsyncWrap since the point it captures at was already on the native side.

src/async_wrap.cc

Co-authored-by: Anna Henningsen <github@addaleax.net>

addaleax

Sorry, just saw your comments on the DCHECK (hidden because of the resolved comment) – that's exactly why the DCHECK should be in place, we should not be able to MakeCallback() when there isn't a corresponding context frame to be entered.

I can try to find some time to dig into those failures.

addaleax · 2026-02-27T16:02:22Z

Yeah, the only error I'm getting is this one:

Path: parallel/test-http-req-close-robust-from-tampering
#  out/Release/node[96212]: MaybeLocal<Value> node::AsyncWrap::MakeCallback(const Local<Function>, int, Local<Value> *) at ../src/async_wrap.cc:665
  #  Assertion failed: !context_frame_.IsEmpty()

----- Native stack trace -----

 1: 0x104c41c8c node::DumpNativeBacktrace(__sFILE*) [[...]/src/node/out/Release/node]
 2: 0x104dbf8b8 node::Assert(node::AssertionInfo const&) [[...]/src/node/out/Release/node]
 3: 0x104bb7630 node::AsyncWrap::MakeCallback(v8::Local<v8::Function>, int, v8::Local<v8::Value>*) [[...]/src/node/out/Release/node]
 4: 0x104dfe5f0 node::http_parser::Parser::on_body(char const*, unsigned long) [[...]/src/node/out/Release/node]
 5: 0x104df637c node::http_parser::Parser::Proxy<int (node::http_parser::Parser::*)(char const*, unsigned long), &node::http_parser::Parser::on_body(char const*, unsigned long)>::Raw(llhttp__internal_s*, char const*, unsigned long) [[...]/src/node/out/Release/node]
 6: 0x106ba6300 llhttp__on_body [[...]/src/node/out/Release/node]
 7: 0x106ba3f68 llhttp__internal__run [[...]/src/node/out/Release/node]
 8: 0x106b9e160 llhttp__internal_execute [[...]/src/node/out/Release/node]
 9: 0x104dfb064 node::http_parser::Parser::Execute(char const*, unsigned long) [[...]/src/node/out/Release/node]
10: 0x104dfa188 node::http_parser::Parser::OnStreamRead(long, uv_buf_t const&) [[...]/src/node/out/Release/node]
11: 0x104ccc25c node::StreamResource::EmitRead(long, uv_buf_t const&) [[...]/src/node/out/Release/node]
12: 0x105033a58 node::LibuvStreamWrap::OnUvRead(long, uv_buf_t const*) [[...]/src/node/out/Release/node]
13: 0x105034ba0 node::LibuvStreamWrap::ReadStart()::$_1::operator()(uv_stream_s*, long, uv_buf_t const*) const [[...]/src/node/out/Release/node]
14: 0x105034b30 node::LibuvStreamWrap::ReadStart()::$_1::__invoke(uv_stream_s*, long, uv_buf_t const*) [[...]/src/node/out/Release/node]
15: 0x1065023c4 uv__stream_io [[...]/src/node/out/Release/node]
16: 0x10650a980 uv__io_poll [[...]/src/node/out/Release/node]
17: 0x1064f6e88 uv_run [[...]/src/node/out/Release/node]
[...]
Command: out/Release/node [...]/src/node/test/parallel/test-http-req-close-robust-from-tampering.js
--- CRASHED (Signal: 6) ---

That's the test from #37958, which was added to complete coverage and intentionally uses internal APIs.

We could skip the DCHECK for now, but it does catch a valid issue here I'd say.

Flarna · 2026-02-28T00:15:30Z

Setting undefined in AsyncWrap::EmitDestroy() should be fine as it fulfills the contract to have a Local<Value> in MakeCallback. If no context frame was ever set (e.g. no ALS user) the context frame it will be also undefined.

The failing test shows actually a race in HTTP:

The http server socket receives the complete HTTP message in a single TCP package.
HTTP parser parses headers resulting in emiting request event.
During the request event the parser is freeded.
once the function returns back to the llhttp parser that one happily continues to process the tcp packet and emits the body - on the already freed parser.

Adding parser[kOnBody] = null; here in cleanParser() avoids the call to onBody but the race that a freed parser is still used is not solved.

But well, thats an unrelated issue for an http expert.

Do you want me to remove the DCHECK? I think it doesn't harm.

addaleax · 2026-02-28T21:08:03Z

Do you want me to remove the DCHECK? I think it doesn't harm.

No. We should have something that tells us if the behavior is broken that way. c458399 only fixed the test by breaking the DCHECK's functionality, but its purpose is still valid.

I'm kind of leaning towards something like

#ifdef DEBUG
  if (
    context_frame_.IsEmpty() ||
    context_frame_.Get(env()->isolate())->IsUndefined()
  ) {
    ProcessEmitWarning(env(), "MakeCallback() called without context frame");
  }
#endif

with the goal of eventually turning it into a regular DCHECK() or even CHECK() again.

Flarna · 2026-03-01T17:08:20Z

I don't think emitting a warning in case context_frame_ == undefined is a good idea as this would issue warnings on default. At least I get a warning in repl on every typed character.

async_context_frame::current() is just a wrapper of isolate->GetContinuationPreservedEmbedderDataV2() which returns undefined if noone set anything before via ALS, see here.
If an empty Local<Value> is passed to SetContinuationPreservedEmbedderDataV2() it creates undefined internally.

The ALS implementation is handling undefined like an empty frame, see here.

We could move towards setting some root_context_frame during startup and expect/hope that v8 never looses context but I think this is out of scope in this PR.
We could also improve the naming a bit to reflect that context_frame_ might be actually undefined.

What about adding following and reverting c458399 ?

#ifdef DEBUG
  if (context_frame_.IsEmpty()) {
    ProcessEmitWarning(env(), "MakeCallback() called without context_fream, likely use after destroy of AsyncWrap");
  }
#endif

addaleax · 2026-03-02T10:23:35Z

What about adding following and reverting c458399 ?

Yeah, that would work then 👍

nodejs-github-bot · 2026-03-03T05:16:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/71538/

nodejs-github-bot · 2026-03-03T06:50:41Z

Commit Queue failed

- Loading data for nodejs/node/pull/61995
✔  Done loading data for nodejs/node/pull/61995
----------------------------------- PR info ------------------------------------
Title      src: release context frame in AsyncWrap::EmitDestroy (#61995)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     Flarna:als-http-parser-leak -> nodejs:main
Labels     c++, author ready, needs-ci, commit-queue-squash, dont-land-on-v20.x, async_local_storage
Commits    4
 - src: release context frame in AsyncWrap::EmitDestroy
 - Add DCHECK to ensure context_frame is not emtpy in MakeCallback
 - use undefined to avoid empty global
 - set empty context_frame_, issue a process warning
Committers 2
 - Gerhard Stöbich <18708370+Flarna@users.noreply.github.com>
 - GitHub <noreply@github.com>
PR-URL: https://github.com/nodejs/node/pull/61995
Fixes: https://github.com/nodejs/node/issues/61882
Refs: https://github.com/nodejs/node/pull/48528
Reviewed-By: Anna Henningsen <anna@addaleax.net>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/61995
Fixes: https://github.com/nodejs/node/issues/61882
Refs: https://github.com/nodejs/node/pull/48528
Reviewed-By: Anna Henningsen <anna@addaleax.net>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 26 Feb 2026 00:07:32 GMT
   ✔  Approvals: 1
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/61995#pullrequestreview-3879134622
   ✘  This PR needs to wait 42 more hours to land (or 0 minutes if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2026-03-03T05:16:27Z: https://ci.nodejs.org/job/node-test-pull-request/71538/
- Querying data for job/node-test-pull-request/71538/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/22611759900

Qard

LGTM. As for the note about v8::Global versus internal field: there's no particular reason it is the way it is. Just seemed more straightforward at the time since the frame value is initially captured from the native side. Simply adding it to the AsyncWrap rather than the wrapped JS object itself seemed the most straightforward thing to do at the time.

Qard · 2026-03-03T13:09:04Z

src/async_wrap.cc

+      HandleScope handle_scope(env()->isolate());
+      USE(object()->Set(env()->context(), env()->resource_symbol(), object()));
+    }
+    context_frame_.Reset();


No particular reason, I guess. Probably could be an internal field. Just needed to have a context frame reference somewhere related to an AsyncWrap so it could do the restore between the before and after. It was just easier to store directly on the AsyncWrap since the point it captures at was already on the native side.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 26, 2026

Flarna added the async_local_storage AsyncLocalStorage label Feb 26, 2026

Flarna mentioned this pull request Feb 26, 2026

AsyncContextFrame may not be released when using HTTP parsers #61882

Open

src: release context frame in AsyncWrap::EmitDestroy

bdd2d38

Release the async context frame in AsyncWrap::EmitDestroy to allow gc to collect it. This is in special relevant for reused resources like HTTPParser otherwise they might keep ALS stores alive.

Flarna force-pushed the als-http-parser-leak branch from 391ae5a to bdd2d38 Compare February 26, 2026 00:27

Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2026

addaleax reviewed Feb 26, 2026

View reviewed changes

Add DCHECK to ensure context_frame is not emtpy in MakeCallback

2500801

Co-authored-by: Anna Henningsen <github@addaleax.net>

Flarna added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 26, 2026

use undefined to avoid empty global

c458399

Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2026

addaleax approved these changes Feb 27, 2026

View reviewed changes

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2026

addaleax requested changes Feb 27, 2026

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2026

This comment was marked as outdated.

Sign in to view

set empty context_frame_, issue a process warning

3b54b9a

Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2026

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 2, 2026

This comment was marked as outdated.

Sign in to view

Flarna requested a review from addaleax March 2, 2026 20:22

addaleax approved these changes Mar 2, 2026

View reviewed changes

addaleax added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Mar 2, 2026

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2026

This comment was marked as outdated.

Sign in to view

Flarna added commit-queue Add this label to land a pull request using GitHub Actions. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Mar 3, 2026

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 3, 2026

Flarna added the review wanted PRs that need reviews. label Mar 3, 2026

Qard approved these changes Mar 3, 2026

View reviewed changes

Uh oh!

Conversation

Flarna commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

addaleax Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Flarna Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Qard Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

addaleax left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

addaleax commented Feb 27, 2026

Uh oh!

Flarna commented Feb 28, 2026

Uh oh!

addaleax commented Feb 28, 2026

Uh oh!

Flarna commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Mar 2, 2026

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Mar 3, 2026

Uh oh!

nodejs-github-bot commented Mar 3, 2026

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Qard Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Flarna commented Feb 26, 2026 •

edited

Loading

codecov bot commented Feb 26, 2026 •

edited

Loading

Qard Mar 3, 2026 •

edited

Loading

addaleax left a comment •

edited

Loading

Flarna commented Mar 1, 2026 •

edited

Loading

Qard Mar 3, 2026 •

edited

Loading